Enhance onchain transaction management #628

Camillarhi · 2025-08-23T16:45:37Z

This PR enhances on-chain transaction management:

Rebroadcast/bumping of on-chain wallet transactions
Handle RBF'd Pending payments

Changes

Added background job for rebroadcasting unconfirmed onchain transactions with max attempt limit
Implemented RBF (Replace-by-Fee) functionality allowing users to bump fees on outbound unconfirmed transactions

Related Issues

Fixes Rebroadcast/bumping of on-chain wallet transactions #367

ldk-reviews-bot · 2025-08-23T16:45:40Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

ldk-reviews-bot · 2025-08-25T16:56:52Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-08-27T16:57:27Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-08-30T00:00:58Z

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-01T00:01:42Z

🔔 4th Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Just took an initial look and added a few high-level comments. I have yet to review any of the actual RBF logic changes.

However, as noted elsewhere, please don't make all the changes as single huge commit, but rather break the PR up in logical commits that all have descriptive commit messages outlining what the change is, why it's necessary, etc. For guidance you can have a look at https://cbea.ms/git-commit/

It would also make sense to not include the changes for #452 in this initial PR directly, but do it in a separate PR to keep the diff more manageable and reviewable.

bindings/ldk_node.udl

src/payment/onchain.rs

src/payment/store.rs

moisesPompilio · 2025-09-05T02:36:39Z

Thanks for the PR.
It might be better to split this into at least two commits, one for rebroadcasting and one for RBF, to improve organization and reviewability.

moisesPompilio · 2025-09-05T02:28:37Z

src/wallet/mod.rs

+		self.payment_store.remove(&payment_id)?;
+
+		self.payment_store.insert_or_update(payment_details)?;


I'm not fully sure about this flow: since with RBF we can’t guarantee which transaction will eventually confirm (usually the last one, but not always), removing the previous payment here might cause issues. What happens if the earlier tx ends up confirming instead of the latest — would the wallet balance be deducted while the store still shows the payment as pending? If so, could this lead to a confusing UX for the user?

Excellent catch. You're right to be concerned about the potential for state inconsistency.

However, the update_payment_store method acts as a reconciler that syncs the payment store with the actual state of the BDK wallet. Since the payment store is designed to be a reflection of the wallet's state.

We might potentially still end up with multiple entries for the same payment though, right, as we'd not drop the RBF entry once the original transaction confirms? There would also be no 'history' of all the bumps that happened.

Yes, having multiple entries is a possibility. Regarding dropping the entry once the original confirms, that's what I plan on doing in this issue #452 .

As for the history of bumps, a solution could be introducing another status replaced with a replacement ID that indicates the transaction was bumped and replaced.

I think one way to simplify tracking RBF transactions is to add two fields to each payment: is_rbf (a boolean indicating whether the transaction is part of an RBF sequence) and origin_payment_id (optional, pointing to the original payment in the sequence). The original transaction would have is_rbf = true and origin_payment_id = None. Each RBF attempt would also have is_rbf = true and set origin_payment_id to the original transaction’s ID. If a new RBF is created from an existing RBF, it can reuse the same origin_payment_id, linking it back to the original.

With this setup, handling confirmations becomes straightforward:

If an RBF transaction confirms, find the original transaction via origin_payment_id and clean up all other RBFs linked to it.

When a transaction with is_rbf = true and origin_payment_id = None (i.e., an original transaction) confirms, find all RBF transactions with origin_payment_id equal to its ID and remove them.

This keeps data consistent and ensures that extra lookups are only needed when is_rbf is true, making RBF handling explicit and efficient.

For example, with A as the original transaction and B, C, D as RBFs:

graph TD A[A is_rbf=true origin=None] B[B is_rbf=true origin=A.payment_id] C[C is_rbf=true origin=A.payment_id] D[D is_rbf=true origin=A.payment_id] A --> B A --> C A --> D

Loading

I think there’s a design consideration for transactions that won’t confirm because an RBF replaced them. When an RBF transaction confirms, the original and any other RBFs in the sequence become invalid. In my opinion, a more user-friendly approach is to mark them as failed and reference the txid of the confirming transaction, which makes it clear why these transactions didn’t confirm while keeping the history intact.

Thank you for this feedback. This aligns well with the introduction of a replacement id as mentioned above.

Regarding retaining the history of bumped transactions and marking them as failed when any confirms, this could lead to increasing clutter of failed transactions if RBF is used frequently.

My proposed approach during node sync and store updates is when a confirmed transaction is encountered, if it's an RBF transaction, we can confirm that transaction and clear/remove the history of bumps from the store. This prevents unnecessary accumulation of failed transaction records while maintaining proper transaction state tracking.

Camillarhi · 2025-09-05T13:19:14Z

Thanks for the PR. It might be better to split this into at least two commits, one for rebroadcasting and one for RBF, to improve organization and reviewability.

Thanks for the review! I've split the PR into two separate commits as suggested

ldk-reviews-bot · 2025-09-06T00:00:58Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-08T00:01:42Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-10T00:02:19Z

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Did another pass and added some comments. Still have to take an even closer look at the RBF logic and think through edge cases.

src/config.rs

src/builder.rs

src/config.rs

src/lib.rs

src/payment/onchain.rs

src/payment/store.rs

src/wallet/mod.rs

tnull · 2025-09-10T12:38:22Z

src/wallet/mod.rs

+		self.payment_store.remove(&payment_id)?;
+
+		self.payment_store.insert_or_update(payment_details)?;


We might potentially still end up with multiple entries for the same payment though, right, as we'd not drop the RBF entry once the original transaction confirms? There would also be no 'history' of all the bumps that happened.

ldk-reviews-bot · 2025-09-20T00:00:40Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-22T00:01:50Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-24T00:02:44Z

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

@Camillarhi Excuse the delay here - last few weeks have been super busy pushing towards an LDK 0.2 beta release. Will see to get back to review on this in a proper ASAP, it might take another week or so though.

In the meantime I want to note that recently there has been some movement towards #446 as BDK now added events (see bitcoindevkit/bdk_wallet#6 / bitcoindevkit/bdk_wallet#310) which will ship as part of the next BDK 2.2 release. So I do wonder if we should generally first move to update our payment store based on the events emitted from syncing (also allowing for #448 to land), and then basing the changes here on top. This might be in particular interesting as bitcoindevkit/bdk_wallet#310 included a TxReplaced event which might allow us to detect and track (all?) RBF transactions belonging to a 'payment' more easily.

Any thoughts on this?

Camillarhi · 2025-09-26T09:53:41Z

@Camillarhi Excuse the delay here - last few weeks have been super busy pushing towards an LDK 0.2 beta release. Will see to get back to review on this in a proper ASAP, it might take another week or so though.

In the meantime I want to note that recently there has been some movement towards #446 as BDK now added events (see bitcoindevkit/bdk_wallet#6 / bitcoindevkit/bdk_wallet#310) which will ship as part of the next BDK 2.2 release. So I do wonder if we should generally first move to update our payment store based on the events emitted from syncing (also allowing for #448 to land), and then basing the changes here on top. This might be in particular interesting as bitcoindevkit/bdk_wallet#310 included a TxReplaced event which might allow us to detect and track (all?) RBF transactions belonging to a 'payment' more easily.

Any thoughts on this?

Thanks for the update, no problem at all on the review delay. I’ll go through the links you mentioned and follow up with thoughts

Camillarhi · 2025-09-27T01:32:16Z

@Camillarhi Excuse the delay here - last few weeks have been super busy pushing towards an LDK 0.2 beta release. Will see to get back to review on this in a proper ASAP, it might take another week or so though.

In the meantime I want to note that recently there has been some movement towards #446 as BDK now added events (see bitcoindevkit/bdk_wallet#6 / bitcoindevkit/bdk_wallet#310) which will ship as part of the next BDK 2.2 release. So I do wonder if we should generally first move to update our payment store based on the events emitted from syncing (also allowing for #448 to land), and then basing the changes here on top. This might be in particular interesting as bitcoindevkit/bdk_wallet#310 included a TxReplaced event which might allow us to detect and track (all?) RBF transactions belonging to a 'payment' more easily.

Any thoughts on this?

Thanks for the update, no problem at all on the review delay. I’ll go through the links you mentioned and follow up with thoughts

Thanks for the update and for sharing those links. I've reviewed the BDK events implementation, and I agree that using the new events system would be a much cleaner approach for tracking on-chain payment states and handling RBF cases by updating the payment stores with the events emitted.

It makes perfect sense to let #448 land first to establish the new events structure, and then rebase the changes from this PR on top of that foundation.

On a related note, I saw that you opened #488. Would you like me to continue working on that, or would you prefer to handle it yourself? I'm happy to help.

tnull · 2025-10-09T08:07:21Z

Thanks for the update and for sharing those links. I've reviewed the BDK events implementation, and I agree that using the new events system would be a much cleaner approach for tracking on-chain payment states and handling RBF cases by updating the payment stores with the events emitted.

It makes perfect sense to let #448 land first to establish the new events structure, and then rebase the changes from this PR on top of that foundation.

On a related note, I saw that you opened #488. Would you like me to continue working on that, or would you prefer to handle it yourself? I'm happy to help.

So, as mentioned over at #448 and elsewhere, we probably end up delaying #448 a bit more until we get lightningdevkit/rust-lightning#3566 with LDK 0.3. However, we now upgraded to BDK 2.2, i.e., could switch to make use of the events BDK now emits when applying wallet update. This is likely preferable to the current 'always-iterate-all-wallet-transactions' approach we currently use in update_payment_store. Would you be interested in working on a PR that simply migrates the current state over to use the BDK syncing events?

Then, associating new RBF transactions with the pre-existing payment store entry in this PR could become considerably more easy/less error prone, given that the new WalletEvent::TxReplaced variant gives us a list of conflicting transactions: https://docs.rs/bdk_wallet/latest/bdk_wallet/event/enum.WalletEvent.html#variant.TxReplaced

Camillarhi · 2025-10-09T10:17:04Z

So, as mentioned over at #448 and elsewhere, we probably end up delaying #448 a bit more until we get lightningdevkit/rust-lightning#3566 with LDK 0.3. However, we now upgraded to BDK 2.2, i.e., could switch to make use of the events BDK now emits when applying wallet update. This is likely preferable to the current 'always-iterate-all-wallet-transactions' approach we currently use in update_payment_store. Would you be interested in working on a PR that simply migrates the current state over to use the BDK syncing events?

Then, associating new RBF transactions with the pre-existing payment store entry in this PR could become considerably more easy/less error prone, given that the new WalletEvent::TxReplaced variant gives us a list of conflicting transactions: https://docs.rs/bdk_wallet/latest/bdk_wallet/event/enum.WalletEvent.html#variant.TxReplaced

Yeah, that makes a lot of sense! Switching to BDK 2.2’s sync events would clean up update_payment_store significantly, no more iterating through all transactions on every sync, and the TxReplaced event would indeed make handling RBF much more straightforward and reliable.

I’d be happy to take this on. I’ll open a PR to migrate update_payment_store to use BDK’s emitted events instead of the current full-scan approach. That should also set us up nicely for cleaner RBF handling in this PR.

Introduces a `RebroadcastPolicy` to manage the automatic rebroadcasting of unconfirmed transactions with exponential backoff. This prevents excessive network spam while systematically retrying stuck transactions. The feature is enabled by default but can be disabled via the builder: `builder.set_auto_rebroadcast_unconfirmed(false)`. Configuration options: - min_rebroadcast_interval: Base delay between attempts (seconds) - max_broadcast_attempts: Total attempts before abandonment - backoff_factor: Multiplier for exponential delay increase Sensible defaults are provided (300s, 24 attempts, 1.5x backoff).

Add `Replace-by-Fee` functionality to allow users to increase fees on pending outbound transactions, improving confirmation likelihood during network congestion. - Uses BDK's `build_fee_bump` for transaction replacement - Validates transaction eligibility: must be outbound and unconfirmed - Implements fee rate estimation with safety limits - Maintains payment history consistency across wallet updates - Includes integration tests for various RBF scenarios

ldk-reviews-bot · 2026-02-02T00:00:43Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Thanks for the rebase, here are some updated comments.

tnull · 2026-02-02T13:40:34Z

src/wallet/mod.rs

+						None,
+					);

+					self.payment_store.insert_or_update(payment)?;


Hmm, this seems ~unrelated? Is this a fix for something we overlooked/omitted before or do we need this suddenly? If it's the former, this should be in a separate commit.

This should be in the second commit. I'll fix that.

tnull · 2026-02-02T13:43:26Z

src/config.rs

+/// Determines the strategy for resending unconfirmed transactions to the network
+/// to ensure they remain in mempools and eventually get confirmed.
+#[derive(Clone, Debug)]
+pub struct RebroadcastPolicy {


I still don't think we need all this extra boilerplate for something as simple as rebroadcasting. Let's just rebroadcast once a block and be done with it.

Makes sense, I'll remove the RebroadcastPolicy entirely along with the backoff/attempt-tracking logic. I'll trigger the rebroadcast from the ChainTipChanged event from BDK in update_payment_store() instead of the timer-based background task, so it naturally happens once per block.

tnull · 2026-02-02T13:45:44Z

src/payment/pending_payment_store.rs

 	/// Transaction IDs that have replaced or conflict with this payment.
 	pub conflicting_txids: Vec<Txid>,
+	/// The raw transaction for rebroadcasting
+	pub raw_tx: Option<Transaction>,


This is a lot of extra data stored. When would we reliably drop that data again? What happens if this never confirms, will we just keep this entry forever in our store?

I stored raw_tx here so the rebroadcast logic could just call broadcast_transactions() without having to lock and query the BDK wallet. But as you have pointed out, there's no cleanup path if the tx never confirms. I'll drop it from PendingPaymentDetails and pull the transaction from BDK when needed instead.

tnull · 2026-02-02T13:48:29Z

src/wallet/mod.rs

 		None
 	}
+
+	pub(crate) fn rebroadcast_unconfirmed_transactions(&self) -> Result<(), Error> {


As mentioned above, I think all this logic is waaaay to complicated. We should just rebroadcast once a block and be done with it, really doesn't require any user-facing API really, as they need to assume that once-broadcast transaction will remain in/return to the mempool anyways.

tnull · 2026-02-02T13:50:16Z

src/payment/pending_payment_store.rs


 		if let Some(new_conflicting_txids) = &update.conflicting_txids {
-			update_if_necessary!(self.conflicting_txids, new_conflicting_txids.clone());
+			// Don't overwrite existing conflicts with an empty list


Not quite sure I immediately understand why this is happening?

The guard against empty lists was meant to avoid accidentally clearing existing conflict data. I'll clean up the call sites to pass None when there are no conflicts and remove this extra check.

tnull · 2026-02-02T13:54:42Z

src/wallet/mod.rs

 						conflicts.iter().map(|(_, conflict_txid)| *conflict_txid).collect();

+					// Use the last transaction id in the conflicts as the new txid
+					let new_txid = conflicts.last().map(|(_, new_tx)| *new_tx).unwrap_or(txid);


Hmm, this seems very unreliable? I don't think it's part of the API guarantees that it's always the last Txid that will be our original Txid? Can we lean on that or do we need more explicit APIs from BDK here?

From my tests, it happened to be the last one, but looking at the implementation, direct_conflicts used to get the conflicts for a replaced txid, comes from BDK's tx_graph and doesn't guarantee any ordering. I think this needs to be fixed upstream in BDK. What do you think is the best approach here?

tnull · 2026-02-02T13:56:13Z

src/wallet/mod.rs

+
+			match builder.finish() {
+				Ok(psbt) => Ok(psbt),
+				Err(CreateTxError::FeeRateTooLow { required }) => {


Can we name this required_fee_rate or similar?

tnull · 2026-02-02T13:57:23Z

src/wallet/mod.rs

+					log_info!(self.logger, "BDK requires higher fee rate: {}", required);
+
+					// Safety check
+					const MAX_REASONABLE_FEE_RATE_SAT_VB: u64 = 1000;


Where did we get this limit from? Also, usually sat/vb is a float value and we tend to use sat/kwu in LDK. Would be great to keep it as consistent as possible and do such calculations in sat/kwu too.

tnull · 2026-02-02T13:59:05Z

tests/integration_tests_rust.rs

 #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
-async fn test_rbf_via_mempool() {
-	run_rbf_test(false).await;
+async fn onchain_fee_bump_rbf() {


Can we leave the original test in place and add (a lot more) test coverage via proptests or similarly randomized test scenarios? It seems that with RBF so much could go wrong that we need to add considerable test coverage before moving on.

ldk-reviews-bot requested a review from tnull August 23, 2025 16:56

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 39922cc to c228760 Compare August 23, 2025 16:58

Camillarhi mentioned this pull request Aug 26, 2025

Figure out rebroadcasting / RBF strategy #22

Open

tnull reviewed Sep 1, 2025

View reviewed changes

bindings/ldk_node.udl Outdated Show resolved Hide resolved

bindings/ldk_node.udl Outdated Show resolved Hide resolved

src/payment/onchain.rs Outdated Show resolved Hide resolved

src/payment/onchain.rs Outdated Show resolved Hide resolved

src/payment/store.rs Outdated Show resolved Hide resolved

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch 3 times, most recently from a52b1e5 to 3523954 Compare September 3, 2025 22:44

Camillarhi requested a review from tnull September 3, 2025 22:50

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 3523954 to 609d0a0 Compare September 3, 2025 22:53

moisesPompilio reviewed Sep 5, 2025

View reviewed changes

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 609d0a0 to 8b7cf78 Compare September 5, 2025 13:17

tnull reviewed Sep 10, 2025

View reviewed changes

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch 2 times, most recently from 06e7ff3 to 6e6e2aa Compare September 11, 2025 23:09

tnull self-requested a review September 17, 2025 11:11

moisesPompilio mentioned this pull request Sep 23, 2025

Fix Invalid Pending Payments and Strengthen RBF Tests #647

Closed

tnull reviewed Sep 26, 2025

View reviewed changes

tnull mentioned this pull request Oct 9, 2025

Implement CPFP fee bumping for unconfirmed transactions #638

Open

Camillarhi mentioned this pull request Oct 9, 2025

Use BDK events in update_payment_store #658

Merged

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch 5 times, most recently from 5936f63 to 8ec7391 Compare January 30, 2026 08:54

Camillarhi added 2 commits January 30, 2026 10:06

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 8ec7391 to aa1558a Compare January 30, 2026 09:12

Camillarhi requested a review from tnull January 30, 2026 09:13

tnull reviewed Feb 2, 2026

View reviewed changes

		self.payment_store.remove(&payment_id)?;

		self.payment_store.insert_or_update(payment_details)?;

Enhance onchain transaction management #628

Are you sure you want to change the base?

Enhance onchain transaction management #628

Conversation

Camillarhi commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Related Issues

Uh oh!

ldk-reviews-bot commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Aug 25, 2025

Uh oh!

ldk-reviews-bot commented Aug 27, 2025

Uh oh!

ldk-reviews-bot commented Aug 30, 2025

Uh oh!

ldk-reviews-bot commented Sep 1, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moisesPompilio commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Camillarhi commented Sep 5, 2025

Uh oh!

ldk-reviews-bot commented Sep 6, 2025

Uh oh!

ldk-reviews-bot commented Sep 8, 2025

Uh oh!

ldk-reviews-bot commented Sep 10, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Sep 20, 2025

Uh oh!

ldk-reviews-bot commented Sep 22, 2025

Uh oh!

ldk-reviews-bot commented Sep 24, 2025

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Camillarhi commented Sep 26, 2025

Uh oh!

Camillarhi commented Sep 27, 2025

Uh oh!

Camillarhi commented Aug 23, 2025 •

edited

Loading

ldk-reviews-bot commented Aug 23, 2025 •

edited

Loading

tnull left a comment •

edited

Loading